Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Feature/parameter improvements #651

Merged

Conversation

nulinspiratie
Copy link
Contributor

@nulinspiratie nulinspiratie commented Jun 21, 2017

Fixes parameter issues raised in #600 (except CombinedParameter), and maybe also #498, #92. See below for list of open questions

Major parameter changes

  • Remove ManualParameter (it's behaviour can be captured quite easily in Parameter). Keep as deprecated parameter.
  • Remove StandardParameter,, as most of it properties should be used by a wider range of parameters. Its unique feature is connecting a function/VISA command to its get/set operations. This can be incorporated into the Parameter. Keep as deprecated parameter
  • Create wrappers for get/set that ensures additionals such as delays are always included

Other changes

  • Change behaviour of parameter.get_latest(). It now will return the latest value, unless it was obtained longer than max_val_age ago, in which case it will perform a new get(). This ensures that a value is never used that is older than max_val_age. Part of this includes moving max_val_age into GetLatest.
  • Remove max_delay, delay_tolerance as they were not doing anything. If anyone knows what they are supposed to do please let me know
  • Modify get/set_delay into property
  • Change delay into post_delay, i.e. delay (settling time) after each get()
  • Add inter_delay, i.e. minimum delay between successive get() calls
  • Move delays to BaseParameter, as they can be useful for other parameter types as well
  • Change get_step/set_step to property
  • Move step to BaseParameter, as it could also be useful for other parameters (such as CombinedParameter or ManualParameter)
  • Change _set_get/_set_set to _initialize_get/_initialize_set because naming was confusing
  • Combine validate_and_set, validate_and_sweep to set()
  • Add get_sweep_values method to BaseParameter, used when performing a .set. It's default behaviour is to simply return the same value, but if step is defined, it will return a list of sweep values (taken from StandardParameter).
  • Move validate to BaseParameter, as it later might be used by other parameter types as well.
  • Accept numpy arrays as setpoints (Fixes one of the MultiParameter issues in MultiParameter usability issues #498, other issues need to be fixed elsewhere)
  • Add scale, which can scale (convert) its value. This can be handled in the get/set wrappers. Additionally, a raw_value should be an attribute which is included in the metadata
  • Accept numpy array for sweep points. Not sure if this should be via slicing (might be removed SweepValues, Troubles ?  #465), or .sweep, although this might be too long (param.sweep(vals=sweep_vals) vs param[sweep_vals]. Required changing SweepFixedValues
  • Update tests, include new tests.
  • Removal of units in (Array)Parameter, as it was already deprecated
  • Remove full_name, replace by str(parameter). full_name is kept with deprecation warning
  • Combine _latest_val, _latest_ts into _latest dict attr, remove _latest method
  • Remove no_getter/setter, as they are no longer used
  • Only attributes in meta_attrs that are not None are included. As there are now more possible attrs that are by default included, this could otherwise lead to significant overhead when obtaining a snapshot of all parameters.

Open questions

  • Should parameters include logging through the logging module? I'm asking because it might lead to excessive logging (in measurements, parameters can be set hundreds of thousands of times).
  • Should signalling be added to notify parameter event changes (Use events to couple parameters in instrument drivers? #92)? This could have several useful features. One example is that instruments can connect to a parameter, such that if the parameter changes its value, the instrument can raise a flag that it needs to be setup (New "Configuration Parameter"  #377). Using a package like blinker works well for this purpose, but only in a single process (i.e. it does not by itself support multiprocessing). However, some sort of message passing can probably be used to incorporate this. @giulioungaretti do you think this is worthwhile? It can be easily included in the get/set wrapper
  • How can I test for things like delays? Won't including delays in the TestSuite slow down testing?

@giulioungaretti @jenshnielsen @WilliamHPNielsen @MerlinSmiles

Remove max_delay because it did not seem to work.
For more info see bullet point microsoft#600.

This breaks code that used max_delay
Additional changes
- max_val_age is temporarily useless (to be moved to GetLatest)
- creating set val temporarily moved to init

max_val_age temporarily unusable
Max val age is now always checked when get_latest is asked.
A consequence is that parameter.get_latest will perform a get() if the last get was performed more than max_val_age ago.
…rameter.get()

Accidentally removed exception in a previous commit
Still need to update docstrings
Other parameters, such as MultiParameter, may also need some sort of validation.
Will see in the future if validate may instead be an abstract base method that should be overridden
When get_cmd is None and .get is not set, it will be equal to get_latest
When set_cmd is None and .set is not set, it will be equal to saving the value.
This behaviour is equal to the ManualParameter
@giulioungaretti
Copy link
Contributor

hi @nulinspiratie I will let @jenshnielsen and @WilliamHPNielsen take over this :D

@jenshnielsen
Copy link
Collaborator

Seems sensible at a first look but I am a bit worried about removing Manual and StandardParameter there is a lot of code using it both in QCoDeS and in other modules. I think we would at least need to deprecate them for several months before removing them

@nulinspiratie
Copy link
Contributor Author

@WilliamHPNielsen the validator is now set by default to Enum if val mapping is provided.
I modified one last thing: the Parameter used to have vals=Numbers() by default, which I changed to None. I think there will be too many cases when someone wants to use something else, such as a string. This could save quite some headache.

I also updated the docstrings, removed trailing whitespaces etc. I think I've done all I can do now. @jenshnielsen how do we proceed?

@jenshnielsen
Copy link
Collaborator

@nulinspiratie looks good. There is a test for the default validator being number that needs to be changed or removed

@nulinspiratie
Copy link
Contributor Author

@jenshnielsen fixed!

@WilliamHPNielsen
Copy link
Contributor

WilliamHPNielsen commented Sep 29, 2017

Running through the driver examples again to check if stuff works.

Keithley 2600... Passed with warnings (ManualParameter deprecated, full_name deprecated)

Keysight 33500B... Passed with warnings (ManualParameter deprecated)

QDac Channels... Passed with warnings (ManualParameter deprecated, MultiParameters do not support set). Note that the celebrated channel features do not really work [EDIT: Sorry, they do, but the user gets a warning about how they don't]

SR830... Failed because of set_validator in driver.

Tektronix AWG5014C... Passed with warnings (ManualParameter is deprecated)

@WilliamHPNielsen
Copy link
Contributor

WilliamHPNielsen commented Sep 29, 2017

Upshot from the above test: we need to fix the SR830 driver. The channelised QDac warnings are not introduced by this PR, so we should fix them separately.

My verdict: 💃 !
@jenshnielsen?

@jenshnielsen
Copy link
Collaborator

jenshnielsen commented Sep 29, 2017

I would prefer not to break any drivers

Is there any problem with restoring set_validator as deprecated

def set_validator(self, vals):
    """
        Deprecated Set a validator `vals` for this parameter.
            Args:
                vals (Validator):  validator to set

    """
    warnings.warn(
        "set_validator is deprected use `inst.vals = MyValidator` instead")
    if isinstance(vals, Validator):
        self._vals = vals
    else:
        raise TypeError('vals must be a Validator')

I tend to think that Deprecation warnings should not use the logging module but do warnings.warn

@WilliamHPNielsen
Copy link
Contributor

@jenshnielsen, I think it's fine to deprecate as you suggest. Note that self._vals has become self.vals. D'you wanna push that change?

@jenshnielsen
Copy link
Collaborator

Yes let me do that now

@jenshnielsen
Copy link
Collaborator

@WilliamHPNielsen pushed, from my point of view this is ready to land

Copy link
Contributor

@WilliamHPNielsen WilliamHPNielsen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yippie-kay-yay!

@WilliamHPNielsen WilliamHPNielsen merged commit ad4a3ee into microsoft:master Sep 29, 2017
giulioungaretti pushed a commit that referenced this pull request Sep 29, 2017
Author: Serwan Asaad <serwan.asaad@gmail.com>

    Feature/parameter improvements (#651)
giulioungaretti pushed a commit that referenced this pull request Sep 29, 2017
Author: Serwan Asaad <serwan.asaad@gmail.com>

    Feature/parameter improvements (#651)
@nulinspiratie nulinspiratie deleted the feature/parameter_improvements branch November 28, 2017 01:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants